[Merged by Bors] - Add warning when a hierarchy component is missing #5590
Conversation
nicopap
added
C-Docs
nicopap
force-pushed
the
warn-against-missing-computed-visibility
branch
from
a95bc6a
to
b021e81
Compare
| @@ -20,6 +23,29 @@ pub struct DespawnChildrenRecursive { | |||
| pub entity: Entity, | |||
| } | |||
|
|
|||
| /// System to print a warning if at any time we detect an inconsistent hierarchy. | |||
| pub fn hierarchy_healthcheck_system<T: Component>( | |||
There was a problem hiding this comment.
I defined it in the bevy_hierarchy crate to reflect #5507
|
I like this change but did have one comment about the term "HealthCheck" although please take it with a grain of salt; I'm pretty new at looking at Bevy code! To me it's not quite clear from "hierarchy_healthcheck_system" or the VisibilitySystems enum variant "HealthCheck" that it's related to verifying that the entities within a hierarchy are consistent with regards to these specific (hierarchy-affecting?) components. My initial reaction is "what is a healthy hierarchy vs unhealthy?" hierarchy_consistency_check_system and VisibilitySystems::CheckHierarchyConsistency or something like that? Again, it's totally possible HealthCheck is an established pattern that I'm oblivious to though just thought I'd comment and learn |
|
@ramirezmike I actually took the "health check" term from both neovim and my previous job (where we created a tool to check if internal services were online). I do agree that what the system does is not well reflected in the Maybe there is a better name yeT? |
Perhaps we will discover it! hierarchy_component_mismatch_check? |
nicopap
force-pushed
the
warn-against-missing-computed-visibility
branch
from
b021e81
to
26b8f08
Compare
nicopap
requested review from
cart,
ramirezmike and
killercup
and removed request for
ramirezmike and
killercup
|
This feels like the same kind of diagnostic level as system ordering ambiguities, and with the same kind of consequences in case of an issue (it kinda doesn't work but not in an explicit way). What do you think of having a resource |
I like the idea, but I think it should be on by default. Supposedly, the user only benefits from the warning message if they don't know why their code is not working. But adding the resource requires knowing why their code is not working, making the message moot. The resource could be added on |
nicopap
force-pushed
the
warn-against-missing-computed-visibility
branch
from
69184e8
to
6e46fa9
Compare
nicopap
force-pushed
the
warn-against-missing-computed-visibility
branch
from
6e46fa9
to
e5db0f9
Compare
|
Btw I counted three more questions that would have been answered by this diagnostic. |
nicopap
force-pushed
the
warn-against-missing-computed-visibility
branch
from
c1bbab5
to
4617dbf
Compare
A common pitfall since 0.8 is the requirement on `ComputedVisibility` being present on all ancestors of an entity that itself has `ComputedVisibility`, without which, the entity becomes invisible. I myself hit the issue and got very confused, and saw a few people hit it as well, so it makes sense to provide a hint of what to do when such a situation is encountered. We now check that all entities with both a `Parent` and a `ComputedVisibility` component have parents that themselves have a `ComputedVisibility` component. Note that the warning is only printed once. We also add a similar warning to `GlobalTransform`. This only emits a warning. Because sometimes it could be an intended behavior. Alternatives: - Do nothing and keep repeating to newcomers how to avoid recurring pitfalls - Make the transform and visibility propagations tolerant to missing components
Co-authored-by: ira <[email protected]>
Co-authored-by: ira <[email protected]>
nicopap
force-pushed
the
warn-against-missing-computed-visibility
branch
from
4617dbf
to
61a0c75
Compare
IceSentry
added
the
S-Ready-For-Final-Review
# Objective A common pitfall since 0.8 is the requirement on `ComputedVisibility` being present on all ancestors of an entity that itself has `ComputedVisibility`, without which, the entity becomes invisible. I myself hit the issue and got very confused, and saw a few people hit it as well, so it makes sense to provide a hint of what to do when such a situation is encountered. - Fixes #5849 - Closes #5616 - Closes #2277 - Closes #5081 ## Solution We now check that all entities with both a `Parent` and a `ComputedVisibility` component have parents that themselves have a `ComputedVisibility` component. Note that the warning is only printed once. We also add a similar warning to `GlobalTransform`. This only emits a warning. Because sometimes it could be an intended behavior. Alternatives: - Do nothing and keep repeating to newcomers how to avoid recurring pitfalls - Make the transform and visibility propagation tolerant to missing components (#5616) - Probably archetype invariants, though the current draft would not allow detecting that kind of errors --- ## Changelog - Add a warning when encountering dubious component hierarchy structure Co-authored-by: Nicola Papale <[email protected]>
|
Build failed (retrying...): |
# Objective A common pitfall since 0.8 is the requirement on `ComputedVisibility` being present on all ancestors of an entity that itself has `ComputedVisibility`, without which, the entity becomes invisible. I myself hit the issue and got very confused, and saw a few people hit it as well, so it makes sense to provide a hint of what to do when such a situation is encountered. - Fixes #5849 - Closes #5616 - Closes #2277 - Closes #5081 ## Solution We now check that all entities with both a `Parent` and a `ComputedVisibility` component have parents that themselves have a `ComputedVisibility` component. Note that the warning is only printed once. We also add a similar warning to `GlobalTransform`. This only emits a warning. Because sometimes it could be an intended behavior. Alternatives: - Do nothing and keep repeating to newcomers how to avoid recurring pitfalls - Make the transform and visibility propagation tolerant to missing components (#5616) - Probably archetype invariants, though the current draft would not allow detecting that kind of errors --- ## Changelog - Add a warning when encountering dubious component hierarchy structure Co-authored-by: Nicola Papale <[email protected]>
|
Pull request successfully merged into main. Build succeeded: |
bors
bot
changed the title
# Objective A common pitfall since 0.8 is the requirement on `ComputedVisibility` being present on all ancestors of an entity that itself has `ComputedVisibility`, without which, the entity becomes invisible. I myself hit the issue and got very confused, and saw a few people hit it as well, so it makes sense to provide a hint of what to do when such a situation is encountered. - Fixes bevyengine#5849 - Closes bevyengine#5616 - Closes bevyengine#2277 - Closes bevyengine#5081 ## Solution We now check that all entities with both a `Parent` and a `ComputedVisibility` component have parents that themselves have a `ComputedVisibility` component. Note that the warning is only printed once. We also add a similar warning to `GlobalTransform`. This only emits a warning. Because sometimes it could be an intended behavior. Alternatives: - Do nothing and keep repeating to newcomers how to avoid recurring pitfalls - Make the transform and visibility propagation tolerant to missing components (bevyengine#5616) - Probably archetype invariants, though the current draft would not allow detecting that kind of errors --- ## Changelog - Add a warning when encountering dubious component hierarchy structure Co-authored-by: Nicola Papale <[email protected]>
# Objective A common pitfall since 0.8 is the requirement on `ComputedVisibility` being present on all ancestors of an entity that itself has `ComputedVisibility`, without which, the entity becomes invisible. I myself hit the issue and got very confused, and saw a few people hit it as well, so it makes sense to provide a hint of what to do when such a situation is encountered. - Fixes bevyengine#5849 - Closes bevyengine#5616 - Closes bevyengine#2277 - Closes bevyengine#5081 ## Solution We now check that all entities with both a `Parent` and a `ComputedVisibility` component have parents that themselves have a `ComputedVisibility` component. Note that the warning is only printed once. We also add a similar warning to `GlobalTransform`. This only emits a warning. Because sometimes it could be an intended behavior. Alternatives: - Do nothing and keep repeating to newcomers how to avoid recurring pitfalls - Make the transform and visibility propagation tolerant to missing components (bevyengine#5616) - Probably archetype invariants, though the current draft would not allow detecting that kind of errors --- ## Changelog - Add a warning when encountering dubious component hierarchy structure Co-authored-by: Nicola Papale <[email protected]>
# Objective A common pitfall since 0.8 is the requirement on `ComputedVisibility` being present on all ancestors of an entity that itself has `ComputedVisibility`, without which, the entity becomes invisible. I myself hit the issue and got very confused, and saw a few people hit it as well, so it makes sense to provide a hint of what to do when such a situation is encountered. - Fixes bevyengine#5849 - Closes bevyengine#5616 - Closes bevyengine#2277 - Closes bevyengine#5081 ## Solution We now check that all entities with both a `Parent` and a `ComputedVisibility` component have parents that themselves have a `ComputedVisibility` component. Note that the warning is only printed once. We also add a similar warning to `GlobalTransform`. This only emits a warning. Because sometimes it could be an intended behavior. Alternatives: - Do nothing and keep repeating to newcomers how to avoid recurring pitfalls - Make the transform and visibility propagation tolerant to missing components (bevyengine#5616) - Probably archetype invariants, though the current draft would not allow detecting that kind of errors --- ## Changelog - Add a warning when encountering dubious component hierarchy structure Co-authored-by: Nicola Papale <[email protected]>
Objective
A common pitfall since 0.8 is the requirement on
ComputedVisibilitybeing present on all ancestors of an entity that itself has
ComputedVisibility, without which, the entity becomes invisible.I myself hit the issue and got very confused, and saw a few people hit
it as well, so it makes sense to provide a hint of what to do when such
a situation is encountered.
Visibilitycomponent #5616Solution
We now check that all entities with both a
Parentand aComputedVisibilitycomponent have parents that themselves have aComputedVisibilitycomponent.Note that the warning is only printed once.
We also add a similar warning to
GlobalTransform.This only emits a warning. Because sometimes it could be an intended
behavior.
Alternatives:
pitfalls
components (Visibility inheritance should ignore parents without a
Visibilitycomponent #5616)allow detecting that kind of errors
Changelog